fix: stale order map on pooled headerSorter breaks orderless header sorting - #18
Open
burruplambert wants to merge 1 commit into
Open
fix: stale order map on pooled headerSorter breaks orderless header sorting#18burruplambert wants to merge 1 commit into
burruplambert wants to merge 1 commit into
Conversation
headerSorter instances are pooled and shared between SortedKeyValuesBy and SortedKeyValues, but SortedKeyValues never cleared hs.order. A sorter previously used for an ordered sort would keep its order map and apply it to a later orderless sort, producing the wrong header order whenever the new header's lowercased keys collide with entries in the stale order map. Reset hs.order before sorting in SortedKeyValues and add a regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
headerSorterinstances are pooled and shared betweenSortedKeyValuesByandSortedKeyValues, butSortedKeyValuesnever clearshs.order. A sorter previously used for an ordered sort (any header write with aHeader-Order:key) keeps its order map, and a later orderless sort on the same pooled sorter applies that stale order instead of sorting lexicographically.The wrong order shows up whenever the new header's lowercased keys collide with entries in the stale order map - matched keys jump to the front in the stale order's positions while the rest fall back to lexicographic. Since header order is the whole point of this fork, silently reordering headers based on a previous request's order map seems worth fixing.
Changes
header.go- reseths.order = nilinSortedKeyValuesbefore sortingheader_test.go- regression test that runs an ordered sort, returns the sorter to the pool, then verifies an orderless sort is lexicographicNote: #17, which also touched
SortedKeyValuesstate handling, was closed and split into focused PRs (the decompression part is now #19). This fix stands alone either way; a follow-up PR improving the sorter's comparison performance will be cross-referenced here when it opens.